Skip to content

Fix PR#8061 SensorLib nRF ThinkNode M-series - #9084

Merged
thebentern merged 1 commit into
meshtastic:developfrom
vidplace7:fix-nrf-thinknode-sensorlib
Dec 27, 2025
Merged

Fix PR#8061 SensorLib nRF ThinkNode M-series#9084
thebentern merged 1 commit into
meshtastic:developfrom
vidplace7:fix-nrf-thinknode-sensorlib

Conversation

@vidplace7

Copy link
Copy Markdown
Member

Addendum to #8061
Try-fix Elecrow thinknode NRF M-series.

lewisxhe/PCF8563_Library has been replaced with lewisxhe/SensorLib and it seems just these two were missed.

@vidplace7 vidplace7 added the bugfix Pull request that fixes bugs label Dec 26, 2025
@vidplace7

Copy link
Copy Markdown
Member Author

@WillyJL please review 🙏

@WillyJL

WillyJL commented Dec 26, 2025

Copy link
Copy Markdown
Contributor

i dont believe this is the optimal fix: these 2 devices include the library and define PCF8563_RTC, but do not define HAS_RTC. this means they do not actually query the RTC nor use it in any way from what i can understand of the code. this is similar to other things in #8061 where i removed PCF8563 and sensorlib from a lot of devices that included it but didnt use it, because they in fact do not have an RTC and it was just included as copy paste from other variant files.

from what i can tell the correct fix here is to remove sonsorlib for these 2 variants aswell and remove the PCF8563_RTC from their variant.h files.

@WillyJL

WillyJL commented Dec 26, 2025

Copy link
Copy Markdown
Contributor

of course its possible im misunderstanding the code, in that case updating like you did would work yes. but if these devices dont come with an rtc then no need to include the library

@vidplace7
vidplace7 force-pushed the fix-nrf-thinknode-sensorlib branch from 6ca8b40 to 85941bc Compare December 26, 2025 23:02
@vidplace7

Copy link
Copy Markdown
Member Author

i dont believe this is the optimal fix: these 2 devices include the library and define PCF8563_RTC, but do not define HAS_RTC. this means they do not actually query the RTC nor use it in any way from what i can understand of the code. this is similar to other things in #8061 where i removed PCF8563 and sensorlib from a lot of devices that included it but didnt use it, because they in fact do not have an RTC and it was just included as copy paste from other variant files.

from what i can tell the correct fix here is to remove sonsorlib for these 2 variants aswell and remove the PCF8563_RTC from their variant.h files.

I've modified this PR to remove rather than replace 👍

@WillyJL

WillyJL commented Dec 26, 2025

Copy link
Copy Markdown
Contributor

from a quick google it seems like thinknodes come with rtc, but i cant find any info on which rtc they use, and regardless for how the code was setup previously it was not in use anyway, i would assume more work would be needed to make it work.

happy holidays!

@fifieldt fifieldt left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe the lines should be commented with a link to this or other than removed ...

@vidplace7
vidplace7 force-pushed the fix-nrf-thinknode-sensorlib branch 2 times, most recently from e427dba to 388367e Compare December 26, 2025 23:24
@vidplace7

Copy link
Copy Markdown
Member Author

Maybe the lines should be commented with a link to this or other than removed ...

👍 done

Addendum to meshtastic#8061
Try-fix Elecrow thinknode NRF M-series
@vidplace7
vidplace7 force-pushed the fix-nrf-thinknode-sensorlib branch from 388367e to 833dcf2 Compare December 26, 2025 23:26
@thebentern
thebentern merged commit 3473c32 into meshtastic:develop Dec 27, 2025
74 of 75 checks passed
matkam pushed a commit to matkam/meshtastic-firmware that referenced this pull request Dec 29, 2025
horrible-knots pushed a commit to horrible-knots/firmware that referenced this pull request Dec 31, 2025
jeek pushed a commit to jeek/Meshtastic-Exploiteers-Hacker-Pager that referenced this pull request Jun 30, 2026
Itzdavid01 pushed a commit to Itzdavid01/firmware that referenced this pull request Sep 5, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bugfix Pull request that fixes bugs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants